fix!: use the same cell reference constructor in order to ensure consistency #4634

ugur-vaadin · 2023-02-02T08:43:18Z

Description

In Apache POI, CellReference has multiple constructors. In the version that Vaadin V8 used, the constructor CellReference(Cell cell) did not use the sheet name from the Cell object. However, while having the same signature, this constructor started using the sheet name in the new versions. This creates inconsistencies when checking equality for CellReferences since we use different constructors in various locations.

Several options were discussed, including:

Adding cell references with and without the sheet names to the event. This fixes Set.contains usages, however the set will have duplicates and any size comparison would be broken.
Using the cell references that only use the sheet names. This will have the correct size without duplicates, however will require the Set.contains checks to be updated to include the sheet names. This might also be somewhat inconvenient. Using the constructors without sheet names (e.g "B2", "C3") is easier and the sheet name might be expected to determined from the context.
Using the cell references that only use the sheet names internally, but introducing a new class CellSet that handles contains behaviour inside. This will be a breaking change for users that use the set directly or use other Set API.

This PR goes with this option. For the API:

size() is present
contains(...) is present
getCells() is used to get the actual set of cells and perform any other operations

Note that currently, CellSet only supports cells from the same sheet. If necessary, another internal set can be initialized with all the combinations to provide accurate and fast response to contains(...) calls.

Fixes #4588

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

…stency

sonatype-lift · 2023-02-02T09:14:25Z

...readsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/CellSelectionShifter.java

@@ -399,8 +401,10 @@ public void onSelectionDecreasePainted(int r, int c) {
                    if (!SpreadsheetUtil.isCellInRange(selectedCellReference,


NULL_DEREFERENCE: object selectedCellReference last assigned on line 398 could be null and is dereferenced by call to isCellInRange(...) at line 401.

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sissbruecker · 2023-02-06T10:02:37Z

...sheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/command/CellValueCommand.java

@@ -148,7 +148,8 @@ public void captureCellRangeValues(CellRangeAddress... cellRanges) {

    @Override
    public CellReference getSelectedCellReference() {
-        return new CellReference(selectedCellRow, selectedcellCol);
+        return new CellReference(spreadsheet.getActiveSheet().getSheetName(),


For commands, it seems safer to use SpreadsheetCommand.getSheet(). Currently that always delegates to Spreadsheet.getActivesheet(), but there's a chance that the implementation might change at some point.

Addressed in the latest version

sissbruecker · 2023-02-06T10:12:19Z

...adsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/SpreadsheetHandlerImpl.java

@@ -334,7 +334,9 @@ private void fireCellValueChangeEvent(CellRangeAddress region) {
        for (int x = region.getFirstColumn(); x <= region
                .getLastColumn(); x++) {
            for (int y = region.getFirstRow(); y <= region.getLastRow(); y++) {
-                cells.add(new CellReference(y, x));
+                cells.add(new CellReference(


The main issue with the overall change to include spreadsheet names into the cell references is that if a developer is already using cell references without the sheet name, the comparison will now fail. And that's quite easy to achieve, as new CellReference("A2") is a lot easier to use than the other alternatives that include the sheet name. It's very likely a breaking change.

We could consider adding both types of references (with and without sheet name) to the events. For example, for cell value change events we could modify the logic here:

flow-components/vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/CellValueManager.java

Lines 796 to 799 in 245d39b

private void fireCellValueChangeEvent(Set<CellReference> changedCells) {

spreadsheet

.fireEvent(new CellValueChangeEvent(spreadsheet, changedCells));

}

To something like:

private void fireCellValueChangeEvent(Set<CellReference> changedCells) { List<CellReference> cellRefsWithSheetName = changedCells .stream() .filter(ref -> ref.getSheetName() != null) .toList(); cellRefsWithSheetName.forEach(ref -> { CellReference refWithoutSheetName = new CellReference(ref.getRow(), ref.getCol()); changedCells.add(refWithoutSheetName); }); spreadsheet .fireEvent(new CellValueChangeEvent(spreadsheet, changedCells)); }

The downside would be that a change to a single cell will report two changes in the event. Though that might be the lesser evil than relying on which CellReference constructor someone is using.

Addressed in the latest version

…aluechangelistener-for-anything-else-than-text-input

…uechangelistener-for-anything-else-than-text-input

…tener-for-anything-else-than-text-input' of https://github.com/vaadin/flow-components into 4588-spreadsheet-does-not-trigger-addcellvaluechangelistener-for-anything-else-than-text-input

…uechangelistener-for-anything-else-than-text-input

sonarqubecloud · 2023-02-27T18:22:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

…uechangelistener-for-anything-else-than-text-input

sonarqubecloud · 2023-07-18T16:57:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
0.0% Duplication

rolfsmeds · 2023-12-19T08:44:25Z

As we are not aware of any users having problems due to this issue, perhaps it would be better to wait until the next major as it will introduce some breaking changes?

yuriy-fix · 2024-09-03T07:42:34Z

Added to the v25 breaking changes list to be revisited before v25.

fix: use the same cell reference constructor in order to ensure consi…

245d39b

…stency

ugur-vaadin linked an issue Feb 2, 2023 that may be closed by this pull request

Spreadsheet does not trigger addCellValueChangeListener for anything else than text input #4588

Open

ugur-vaadin requested review from yuriy-fix and tomivirkki February 2, 2023 08:43

sonatype-lift bot reviewed Feb 2, 2023

View reviewed changes

sissbruecker reviewed Feb 6, 2023

View reviewed changes

ugur-vaadin and others added 11 commits February 14, 2023 08:12

Merge branch 'master' into 4588-spreadsheet-does-not-trigger-addcellv…

2af83b6

…aluechangelistener-for-anything-else-than-text-input

fix: use getSheet from SpreadsheetCommand to get the sheet

fe708df

fix: apply formatter

6227ec5

fix: add cell reference without sheet name to cell value change event

268ff65

Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…

066e1d2

…uechangelistener-for-anything-else-than-text-input

fix: add missing cell reference without sheet name to event

7105375

Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…

893994a

…uechangelistener-for-anything-else-than-text-input

fix: update cell value change unit test

31fa8ac

Merge branch '4588-spreadsheet-does-not-trigger-addcellvaluechangelis…

8f08024

…tener-for-anything-else-than-text-input' of https://github.com/vaadin/flow-components into 4588-spreadsheet-does-not-trigger-addcellvaluechangelistener-for-anything-else-than-text-input

Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…

9158159

…uechangelistener-for-anything-else-than-text-input

fix: add sheet name to more internal cell references

5476fdd

ugur-vaadin marked this pull request as ready for review February 28, 2023 06:51

ugur-vaadin requested review from vursen and removed request for tomivirkki February 28, 2023 06:52

Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…

09928c6

…uechangelistener-for-anything-else-than-text-input

ugur-vaadin removed request for yuriy-fix and vursen June 30, 2023 05:41

ugur-vaadin marked this pull request as draft June 30, 2023 09:20

ugur-vaadin and others added 2 commits July 3, 2023 20:06

fix: introduce cellset to avoid selected cell duplication in event

530e9e7

Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…

27ce9d9

…uechangelistener-for-anything-else-than-text-input

ugur-vaadin changed the title ~~fix: use the same cell reference constructor in order to ensure consistency~~ fix!: use the same cell reference constructor in order to ensure consistency Jul 3, 2023

ugur-vaadin and others added 2 commits July 10, 2023 17:42

refactor: add contains checks for indexes and add javadocs

f58821b

Merge branch 'main' into 4588-spreadsheet-does-not-trigger-addcellval…

60dd913

…uechangelistener-for-anything-else-than-text-input

refactor: rename api to minimize the need for refactoring

e20b0fe

vursen added the requires new major This would be a breaking change label Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: use the same cell reference constructor in order to ensure consistency #4634

fix!: use the same cell reference constructor in order to ensure consistency #4634

ugur-vaadin commented Feb 2, 2023 •

edited

Loading

sonatype-lift bot Feb 2, 2023

sissbruecker Feb 6, 2023

ugur-vaadin Feb 28, 2023

sissbruecker Feb 6, 2023

ugur-vaadin Feb 28, 2023

sonarqubecloud bot commented Feb 27, 2023

sonarqubecloud bot commented Jul 18, 2023

rolfsmeds commented Dec 19, 2023

yuriy-fix commented Sep 3, 2024

		@@ -399,8 +401,10 @@ public void onSelectionDecreasePainted(int r, int c) {
		if (!SpreadsheetUtil.isCellInRange(selectedCellReference,

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

	private void fireCellValueChangeEvent(Set<CellReference> changedCells) {
	spreadsheet
	.fireEvent(new CellValueChangeEvent(spreadsheet, changedCells));
	}

fix!: use the same cell reference constructor in order to ensure consistency #4634

Are you sure you want to change the base?

fix!: use the same cell reference constructor in order to ensure consistency #4634

Conversation

ugur-vaadin commented Feb 2, 2023 • edited Loading

Description

Type of change

Checklist

sonatype-lift bot Feb 2, 2023

Choose a reason for hiding this comment

sissbruecker Feb 6, 2023

Choose a reason for hiding this comment

ugur-vaadin Feb 28, 2023

Choose a reason for hiding this comment

sissbruecker Feb 6, 2023

Choose a reason for hiding this comment

ugur-vaadin Feb 28, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 27, 2023

sonarqubecloud bot commented Jul 18, 2023

rolfsmeds commented Dec 19, 2023

yuriy-fix commented Sep 3, 2024

ugur-vaadin commented Feb 2, 2023 •

edited

Loading